-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TASK-67] feat: 디자인 시스템 수정 및 추가사항 반영 (색상, 아이콘, Input) #42
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Qodana for JS2 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
Qodana for JS25 new problems were found
☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
Qodana for JS2 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
크리티컬한 점은 아니고 궁금한 점에 대해서 코멘트 달았습니다-!
xl: 'w-15 h-15', | ||
l: 'w-7.5 h-7.5', | ||
xl: 'w-[60px] h-[60px]', | ||
l: 'w-[30px] h-[30px]', | ||
m: 'w-6 h-6', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 얘만 따로 변경사항이 없는데 일부러 남겨두신 걸까요? 해당 크기를 추측하는데는 어려움은 없지만 다른 값들과 포멧이 달라 다른 의도가 있는지 궁금해서 코멘트 남깁니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 ㅋㅋㅋㅋ m 사이즈만 Tailwind CSS
클래스에 등록돼 있어서 커스텀 하지 않고 그대로 작성했습니다! 혹시 포맷이 달라서 불편하시면 알려주셔용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 아닙니다 ㅎㅎ 그대로 가도 좋습니다!
minLength={minLength} | ||
maxLength={maxLength} | ||
endContent={ | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
드문 경우겠지만 endContent의 해당 요소들이 모두 true로서 보이게 되었을때 정렬 문제는 없나요? 요소들이 단순히 태그로 묶여있어서 코멘트 드렸습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 디자인 시스템에 endContent
가 여러 개 적용되는 Input
컴포넌트가 없습니다! 그래서 정렬 문제는 발생하지 않을 듯해여
하지만 향후 endContent
가 여러 개 적용되는 컴포넌트가 생기게 되면 수정하려 합니다 :)
src/components/Input/BasicInput.tsx
Outdated
{isPasswordVisible ? ( | ||
<Icon | ||
name='Eye' | ||
size='m' | ||
className={`text-gray-200 ${value === '' ? 'text-gray-800' : ''}`} | ||
/> | ||
) : ( | ||
<Icon | ||
name='EyeOff' | ||
size='m' | ||
className={`text-gray-200 ${value === '' ? 'text-gray-800' : ''}`} | ||
/> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
궁금한 점인데,
<Icon
name={ isPasswordVisible ? 'Eye' : 'EyeOff' }
size='m'
className={`text-gray-200 ${value === '' ? 'text-gray-800' : ''}`}
/>
이렇게는 구현이 불가능한가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 이런 해당 부분 리팩토링을 깜빡했네여
바로 반영하겠습니다!
📝 작업 내용
#222222
) color를#1B1B1B
로 변경MoreVertical
)Input
: 비즈니스 로직 걷어내서 재사용 가능한 컴포넌트인
BasicInput
으로 변경💬 전달사항
FE & Design 회의 때 추가 요청사항으로 전달받은 '
Input
에 마우스hover
했을 때의 색상을 입력 필드 클릭했을 경우의 색상과 동일하게 처리'는 아직 반영하지 못했습니다.NextUI
의Input
을 사용하다 보니 커스텀이 불가한 부분이 좀 있어서 추후 방법을 찾으면 반영하겠습니다!디자인 시스템 컴포넌트에선 UI만 담당해야 하기 때문에 비즈니스 로직을 걷어내는 과정에서
2-1.
Input
입력 필드에 내용 입력이 안 되는 현상이 생겼습니다.react-hook-form
및 사용하신 라이브러리에 맞춰서 수정해야 하는 부분이라 일단 그대로 두었습니다.2-2.
SignInPage
,SignUpPage
에서 validation 및 메시지 로직을 추가하시거나 수정 부탁드려요.모든 비즈니스 로직은 (리팩토링하는 등) 변형하지 않고 그 코드 그대로
SignInPage
,SignUpPage
에 옮겼습니다.